Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: respect config options in dbt_project.yml #255

Merged
merged 13 commits into from
Dec 28, 2016

Conversation

cmcarthur
Copy link
Member

@cmcarthur cmcarthur commented Dec 23, 2016

Respect config options in dbt_project.yml

Factor out code related to global configuration. Also makes send_anonymous_usage_stats config option work.

@cmcarthur
Copy link
Member Author

Fixes #254

Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a big deal -- good work on the unit test. One comment, but otherwise LGTM


def read_config(profiles_dir=None):
if profiles_dir is None:
profiles_dir = default_profiles_dir
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does default_profiles_dir come from?


def test__explicit_opt_out(self):
self.set_up_config_options(send_anonymous_usage_stats=False)
self.assertFalse(dbt.config.send_anonymous_usage_stats(TMPDIR))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@drewbanin drewbanin assigned cmcarthur and unassigned drewbanin Dec 24, 2016
@cmcarthur
Copy link
Member Author

back atcha @drewbanin

@cmcarthur cmcarthur assigned drewbanin and unassigned cmcarthur Dec 24, 2016
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also use the ~/.dbt/ dir here. Should we use the parsed.profiles_dir value here as well? We shouldn't write a cookie if tracking is turned off either

@@ -5,9 +5,6 @@


def read_config(profiles_dir=None):
if profiles_dir is None:
profiles_dir = default_profiles_dir

path = os.path.join(profiles_dir, 'profiles.yml')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think passing None to os.path.join causes a TypeError.

Previously, this file was assumed to be located at ~/.dbt/profiles.yml. We should either remove the default value for profiles_dir and insist that the caller passes a value, or we should coalesce None to the default filepath

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good call. since read_config is part of the private API of this namespace, and calls have to go through send_anonymous_usage_stats, i just removed the default None value.

@cmcarthur cmcarthur merged commit 2fe3758 into development Dec 28, 2016
@cmcarthur cmcarthur deleted the bugfix/respect-config-options branch December 28, 2016 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants